INFOPLAT-1035: logger implement otelzap#1359
Conversation
|
👋 kirqz23, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
pkg/logger/zap_otel.go
Outdated
|
|
||
| // NewOtelCore initializes an OpenTelemetry Core for exporting logs in OTLP format | ||
| func NewOtelZapCore(loggerProvider otellog.LoggerProvider, opts ...Option) zapcore.Core { | ||
| logger := loggerProvider.Logger(instrumentationLibName) |
There was a problem hiding this comment.
what does this do and is this correct?
iiuc it implies that all logs will refer to github.com/smartcontractkit/chainlink-common as opposed to the actual caller
There was a problem hiding this comment.
totally agree, we should make it configurable for the logging app
There was a problem hiding this comment.
instrumentationLibName should be configurable
Co-authored-by: Pavel <177363085+pkcll@users.noreply.github.com>
pkg/logger/zap_otel.go
Outdated
| type Option func(c *OtelZapCore) | ||
|
|
||
| // NewOtelCore initializes an OpenTelemetry Core for exporting logs in OTLP format | ||
| func NewOtelZapCore(loggerProvider otellog.LoggerProvider, opts ...Option) zapcore.Core { |
There was a problem hiding this comment.
Lets pass logger interface here to avoid indirect dependency
| func NewOtelZapCore(loggerProvider otellog.LoggerProvider, opts ...Option) zapcore.Core { | |
| func NewOtelZapCore(logger otellog.Logger, opts ...Option) zapcore.Core { |
pkg/logger/logger.go
Outdated
| } | ||
|
|
||
| // NewWithOtelZapCore returns a new Logger using an OpenTelemetry Zap Core. | ||
| func NewWithOtelZapCore(loggerProvider otellog.LoggerProvider) Logger { |
There was a problem hiding this comment.
| func NewWithOtelZapCore(loggerProvider otellog.LoggerProvider) Logger { | |
| func NewWithOtelProvider(loggerProvider otellog.LoggerProvider, loggerName string) Logger { |
pkg/logger/logger.go
Outdated
|
|
||
| // NewWithOtelZapCore returns a new Logger using an OpenTelemetry Zap Core. | ||
| func NewWithOtelZapCore(loggerProvider otellog.LoggerProvider) Logger { | ||
| return &logger{zap.New(NewOtelZapCore(loggerProvider)).Sugar()} |
There was a problem hiding this comment.
| return &logger{zap.New(NewOtelZapCore(loggerProvider)).Sugar()} | |
| return &logger{zap.New(NewOtelZapCore(loggerProvider.Logger(loggerName))).Sugar()} |
pkg/logger/otelzap/otelzap.go
Outdated
|
|
||
| // NOTE: [beholder.SetGlobalOtelProviders](https://github.com/smartcontractkit/chainlink-common/blob/27faefc9ce454c8aa2b1b7484e377ea3e8996bba/pkg/beholder/global.go#L50) | ||
| // must be called before using `NewOtelLogger` otherwise otelglobal.GetLoggerProvider() returns noop provider | ||
| func NewOtelLogger(name string) otellog.Logger { |
There was a problem hiding this comment.
What is the expected use of this function? We did a very deliberate conversion away from global logging a long time ago, and we shouldn't encourage that pattern.
Co-authored-by: Jordan Krage <jmank88@gmail.com>
Co-authored-by: Jordan Krage <jmank88@gmail.com>
|
|
||
| // NewWithCores returns a new Logger with one or more zapcore.Core. | ||
| // If multiple cores are provided, they are combined using zapcore.NewTee. | ||
| func NewWithCores(cores ...zapcore.Core) Logger { |
There was a problem hiding this comment.
Would be good to add some tests for it.
There was a problem hiding this comment.
What do we want to test here? This function just returns the logger with zapcore tee functionality
5 participants
This PR introduces
OtelZapCore, a customzapcore.Coreimplementation that emits structured logs to OpenTelemetry using the OTLP protocol.It also integrates this core into the existing
loggerpackage, allowing all logs to be exported via OTEL while preserving existing logging interfaces.